-
Notifications
You must be signed in to change notification settings - Fork 74
jextract: add support for escaping closures #491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ARCHITECTURE.md
Outdated
| │ (.swift) │ ───▶ │ Representation │ ───▶ │ (.java, .swift) │ | ||
| └─────────────────┘ └─────────────────┘ └─────────────────┘ | ||
| Parse ImportedDecls Generator (JNI/FFM) | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to explain wrap-java here too then I suppose. Its workflow is
The other direction, when starting from a Java library and wanting to access it from Swift is handled by the `wrap-java` command, which uses a classpath as its input:
┌──────────────────┐ ┌─────────────────┐ ┌───────────────────────┐
│ Java classpath │ │ Analysis │ │ Generated Code │
│ (.*class) │ ───▶ │ │ ───▶ │ (.swift with @macros) │
└──────────────────┘ └─────────────────┘ └───────────────────────┘
Parse ImportedDecls Generator (JNI/FFM)
ARCHITECTURE.md
Outdated
| let generator = FFMSwift2JavaGenerator(...) | ||
| try generator.generate() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be concerned having actual code snippets in this guide -- they WILL become outdated and confusing. How about we just explain the same with words please?
ARCHITECTURE.md
Outdated
| @@ -0,0 +1,355 @@ | |||
| # Swift-Java Architecture Guide | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please split out this md file addition from the rest of the PR? Many comments here that are should not affect the rest of the PR
CONTRIBUTING.md
Outdated
| ### Development Setup | ||
|
|
||
| ```bash | ||
| # Prerequisites: Java 22+ and Swift 6.0+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Prerequisites: Java 22+ and Swift 6.0+ | |
| # Prerequisites: Java 25+ and Swift 6.2+ |
| let apiParams = functionType.parameters.map({ $0.parameter.renderParameter() }) | ||
|
|
||
| printer.print( | ||
| printer.print( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent by mistake?
| | `@escaping` `Void` closures: `func callMe(_: @escaping () -> ())` | ❌ | ✅ | | ||
| | `@escaping` closures with primitive arguments/results: `func callMe(_: @escaping (String) -> (String))` | ❌ | ✅ | | ||
| | `@escaping` closures with custom arguments/results: `func callMe(_: @escaping (Obj) -> (Obj))` | ❌ | ❌ | | ||
| | Swift type extensions: `extension String { func uppercased() }` | 🟡 | 🟡 |ommit to support primitive escaping closures) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't changing anything about extensions, why this change?
This is added outside the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, a mistake when solving merge conflicts, thank you
| /// ``` | ||
| public final class JNIClosureContext: @unchecked Sendable { // @unchecked Sendable is needed because the globalRef is a jobject | ||
| /// The JNI GlobalRef to the Java closure object | ||
| public let globalRef: jobject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the existing JavaObjectHolder, no need to introduce a new type
| """ | ||
| public static class setCallback { | ||
| @FunctionalInterface | ||
| public interface callback extends AutoCloseable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is illegal Java code and would not compile, a functional interface cannot have multiple abstract methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the generated code is more correct actually, the test isn't passing and you need to remove those auto closable ideas from here
| printer.print("fatalError(\"Failed to get JNI environment for escaping closure call\")") | ||
| printer.outdent() | ||
| printer.print("}") | ||
| printer.print("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we do a big print with """ strings rather than this style when possible
98efb49 to
7f81ac0
Compare
7f81ac0 to
10282a7
Compare
|
Awesome work 👍 I had a thought about how we could approach this feature. It shares a lot of similarities with the protocol callback work I did in #449 - both escaping closures and protocols are a way to achieve callbacks. Consider the following API func f(closure: @escaping (Int64, MyClass) -> Void)technically we could express this as: protocol f_closure {
func apply(_ arg0: Int64, _ arg1: MyClass)
}
func f(_ closureObject: f_closure) {
self.f {
closureObject.apply($0, $1)
}
}then if we could pass the interface f_closure {
void apply(long arg0, MyClass arg1)
}
private static void f(f_closure_protocol closure_protocol) {
// Downcall to Swift that takes in the protocol
$f(closure_protocol)
}and the required Swift wrappers needed to call that interface from Swift. @FunctionalInterface
interface f_closure {
void apply(long arg0, MyClass arg1)
}
public static void f($f_closure closure) {
// Call the`f` that takes in the "f_closure" Swift protocol:
f($f_closure);
}Then we basically have achieved support for escaping closures, by "proxying" them through protocols. This means we don't have to do double the work when we need to add new features to either protocols or closures. For example, this would give us support for non-primitive types right away, since protocols already support them. Let me know what you think! |
This PR introduces support for escaping closures with primitive types only.